Fix crash caused by repeatedly cloning the same path hierarchy node #1220
Conversation
rdar://150706871
|
@swift-ci please test |
|
@swift-ci please test |
| // FIXME: | ||
| // This code path is both expected (when `knownDisambiguatedPathComponents` is non-nil) and unexpected (when the symbol graph is missing data or contains extra relationships). | ||
| // It would be good to restructure this code to better distinguish what's supported behavior and what's a best-effort attempt at gracefully handle invalid symbol graphs. | ||
| if let existing = parent.children[components.first!] { |
There was a problem hiding this comment.
accessing parents.children without checking parens exists, may be the check is somewhere else ?
There was a problem hiding this comment.
I don't understand at al what this comment is referring to. The parent is not optional so it doesn't make any sense to check if it's nil. The compiler already guarantees that and trying to check using either if let or optional chaining would introduce a compiler error.
The only unwrapped optional value on this line of code is components.first! but that's not mentioned in your comment. Besides, it's safe to access the first element of the components array because this line of code is inside a loop over the components. If the execution enters the loop body it's because the components is non-empty (which makes it perfectly safe to unwrap the first component).
There was a problem hiding this comment.
"accessing parents.children" I meant accessing its element by a specific value, and it is "components.first!" Yes you are right I didn't mention that but that's what I meant. I am just use to not using "!" whenever possible and hence my comment. I understand it would be safe to access it considering where it is used inside the loop over the components.
There was a problem hiding this comment.
Should this index be using component instead of components.first!? It just looks like we're checking the first path component over and over instead of actually looking for successive children of the parent. Ditto for the assertion underneath this block, which i edited in #1212.
|
@swift-ci please test |
|
@d-ronnqvist Thank you for taking time to answer pr comments. |
| // FIXME: | ||
| // This code path is both expected (when `knownDisambiguatedPathComponents` is non-nil) and unexpected (when the symbol graph is missing data or contains extra relationships). | ||
| // It would be good to restructure this code to better distinguish what's supported behavior and what's a best-effort attempt at gracefully handle invalid symbol graphs. | ||
| if let existing = parent.children[components.first!] { |
There was a problem hiding this comment.
Should this index be using component instead of components.first!? It just looks like we're checking the first path component over and over instead of actually looking for successive children of the parent. Ditto for the assertion underneath this block, which i edited in #1212.
|
@swift-ci please test |
d-ronnqvist
dismissed
QuietMisdreavus’s stale review
The blocking feedback has been addressed in 2cdc04c
|
@swift-ci please test |
…wiftlang#1220) * Don't repeatedly clone nodes that have already been cloned rdar://150706871 * Fix unrelated code warning about unused local variable * Fix unrelated list of symbols in other assertion message * Avoid repetition in debug assertions * Only create sparse nodes after processing all symbol graph files * Try to find existing nodes before creating sparse nodes rdar://148247074 * Remove trailing commas for compatibility before Swift 6.1 * Remove unintentional print statement * Try to repair symbol graphs with deep hierarchies but no actual memberOf relationships * Add additional test assertion about counterpart references
…1220) (#1226) * Don't repeatedly clone nodes that have already been cloned rdar://150706871 * Fix unrelated code warning about unused local variable * Fix unrelated list of symbols in other assertion message * Avoid repetition in debug assertions * Only create sparse nodes after processing all symbol graph files * Try to find existing nodes before creating sparse nodes rdar://148247074 * Remove trailing commas for compatibility before Swift 6.1 * Remove unintentional print statement * Try to repair symbol graphs with deep hierarchies but no actual memberOf relationships * Add additional test assertion about counterpart references
Bug/issue #, if applicable: rdar://150706871&148247074
Summary
This primarily fixes a crash caused by repeatedly cloning the same path hierarchy node. (e161b2a)
Then if fixes additional crashes that were made more common due to cloned nodes (4187f62 and c12c7d1)
Seeing these changes in aggregate I would like to take some time to reorganize the path hierarchy initializer to better distinguish the expected code paths from the code paths that try to gracefully handle incomplete symbol graph files. However, I would want to cherry-pick these crash fixes into the 6.2 release and I'm not yet sure about the scope of the other code cleanup so I'd prefer to open a separate PR for that.
Dependencies
None.
Testing
Because most of these issues are related to unexpectedly incomplete symbol graph files it's fairly difficult to test this all the way from symbol graph generation. I could manually craft a symbol graph file that is based on one of the added unit tests, but I don't find that that would provide any more assurances than running the tests which the CI already does.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded